Implement the Mapping interface for GroupingLatestValueCache
#430
Conversation
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR enhances GroupingLatestValueCache by fully adopting the Mapping interface, updating the get method’s behavior, and reflecting those changes in tests and release notes.
- Implement
Mappingmethods (__getitem__,__iter__,__len__) and overloadget - Change
getto return a default/Noneinstead of raisingValueError, and switch toKeyErrorin__getitem__ - Update integration tests and release notes to match the new interface
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_grouping_latest_value_cache_integration.py | Adapted tests to expect get returning None or provided default instead of raising |
| src/frequenz/channels/experimental/_grouping_latest_value_cache.py | Added Mapping inheritance, mapping methods, overloads, and switched exception types |
| RELEASE_NOTES.md | Noted the addition of Mapping interface in release notes |
Comments suppressed due to low confidence (2)
tests/test_grouping_latest_value_cache_integration.py:25
- Consider adding tests for the new Mapping interface methods (
__getitem__,__len__, and iteration) to ensure full coverage of theGroupingLatestValueCachebehavior.
assert cache.get(0) is None
RELEASE_NOTES.md:13
- Add a note under a ‘Breaking Changes’ section to document that
get()no longer raisesValueError(now returnsNoneor a provided default) and that__getitem__raisesKeyErrorfor missing keys.
- This release introduces the experimental `GroupingLatestValueCache`. It is similar to the `LatestValueCache`, but accepts an additional key-function as an argument, which takes each incoming message and returns a key for that message. The latest value received for each unique key gets cached and is available to look up on-demand through a `collections.abc.Mapping` interface.
There was a problem hiding this comment.
Not sure if it is worth keeping c.has_key(k) instead of requiring the use of k in c.
Also not sure if it might be worth implementing also the methods automatically mixed in by Mapping and its subclasses: __contains__, items, values, __eq__, and __ne__, otherwise they are probably calculated via the iterator or something like that, which could be pretty inefficient.
| T_co = typing.TypeVar("T_co", covariant=True) | ||
| T = typing.TypeVar("T") | ||
| HashableT = typing.TypeVar("HashableT", bound=typing.Hashable) |
There was a problem hiding this comment.
Optional: Document these (adding a docstring below them), so they are shown in the docs index.
There was a problem hiding this comment.
They are internal, not exposed from the _grouping_latest_value_cache module. Not sure if they should be though.
There was a problem hiding this comment.
I guess both are fine. If they are not exported, the docs will show it as a "string" without any links, and it will not be present in the ToC. If the types are self explanatory we can keep it as it is. If they are not documented/exported, users will not know about the possible bound= arguments or constraints (or if it is co/contravariant, but for those we usually use the suffixes), so if it is important to the user to know any of that info, that could be a good reason to export and document.
There was a problem hiding this comment.
I've added a commit to do this. I'm somewhat doubtful about exposing them from the experimental package directly, but that seems to be the best option available.
src/frequenz/channels/experimental/_grouping_latest_value_cache.py
Outdated
Show resolved
Hide resolved
All done. |
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Making the part of the public API makes it easy for users to look them up and understand how to use them. Signed-off-by: Sahas Subramanian <[email protected]>
2 participants
No description provided.